fix: prevent false unsaved changes notification when mode/API changes externally #7418
Conversation
… externally - Added logic to detect and handle external changes (from main interface) - External changes to mode or API configuration no longer mark settings as dirty - Only changes made within the Settings dialog itself trigger unsaved changes warning - Fixes #7417
dosubot
bot
added
size:S
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| extensionState.mode !== cachedState.mode || | ||
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) | ||
|
|
||
| if (isExternalChange && !isChangeDetected) { |
There was a problem hiding this comment.
Is this intentional? The condition isExternalChange && !isChangeDetected could lead to race conditions if internal changes happen simultaneously with external changes. Consider using a more robust state management approach or a ref to track the source of changes.
| // by comparing the extension state with our cached state | ||
| const isExternalChange = | ||
| extensionState.mode !== cachedState.mode || | ||
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) |
There was a problem hiding this comment.
Using JSON.stringify for deep comparison on every render could be expensive for large API configurations. Could we use a more efficient deep comparison utility like lodash's isEqual or memoize this comparison?
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) | |
| JSON.stringify(extensionState.apiConfiguration) !== JSON.stringify(cachedState.apiConfiguration) |
| // Check if the changes are coming from outside the settings dialog | ||
| // by comparing the extension state with our cached state | ||
| const isExternalChange = | ||
| extensionState.mode !== cachedState.mode || |
There was a problem hiding this comment.
The code only checks for mode and apiConfiguration changes, but other fields could also change externally (e.g., language, experiments). Should we expand this to cover all fields that could change externally, or is there a specific reason to limit it to these two?
| setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) | ||
| prevApiConfigName.current = currentApiConfigName | ||
| // Reset change detection when API config changes externally | ||
| setChangeDetected(false) |
There was a problem hiding this comment.
There are now three separate places handling similar logic for setChangeDetected(false) (lines 200, 208, and within the new effect). Could we consolidate this logic into a single helper function or custom hook to improve maintainability?
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
|
Closing this PR as it addresses the symptoms but not the root cause. The real issue appears to be that something in the configuration object (particularly with OpenRouter APIs) is changing unexpectedly or not being saved properly, which triggers the dirty state incorrectly. We need to investigate the actual root cause instead of masking the problem. |
github-project-automation
bot
moved this from PR [Needs Prelim Review]
to Done
in Roo Code Roadmap
daniel-lxs
deleted the
fix/issue-7417-invalid-unsaved-changes-notification
branch
Description
This PR fixes issue #7417 where users were incorrectly prompted about unsaved changes when:
Problem
The Settings dialog was incorrectly detecting external changes (mode/API config changes from the main interface) as internal changes, causing a false positive "unsaved changes" warning.
Solution
Added logic to detect and handle external changes separately from internal settings changes:
useEffecthook that monitors for external state changes and syncs them without triggering change detectionTesting
Related Issue
Fixes #7417
Important
Fixes false unsaved changes notification in
SettingsView.tsxby correctly handling external mode/API changes.SettingsView.tsxwhen mode/API changes externally.useEffecthook inSettingsView.tsxto monitor and sync external state changes without triggering change detection.This description was created by
for f9cf6e5. You can customize this summary. It will automatically update as commits are pushed.